Fix SocketStream.aclose() abort() race on the asyncio backend - #1300
Closed
afonsojanu wants to merge 1 commit into
Closed
Fix SocketStream.aclose() abort() race on the asyncio backend#1300afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
If connection_lost() fires while aclose() is suspended at the checkpoint between transport.close() and transport.abort(), the transport has already detached from the event loop by the time control returns. Calling abort() on it then raises AttributeError instead of being the no-op it's meant to be in that interleaving. StreamProtocol now tracks whether connection_lost() has already run, and aclose() skips the abort() call when it has. Fixes agronholm#1250
Owner
|
There are now tons of PRs on this same subject, and I'm drowning in them. Justify why you had to send yet another one. |
Author
|
Fair question. I went back and checked, and #1255 already covers this exact bug (same issue #1250, same fix approach: track connection-lost state on the protocol and skip abort() when it's already true). It's been open since July 30th, so it predates mine by a month. I should have searched open PRs against issue #1250 before starting instead of just checking for an existing fix on master. Closing this one, sorry for adding to the pile. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NOTE Erasing or replacing the contents of this template will result in your pull
request being summarily closed without consideration!
Changes
Fixes #1250.
On the asyncio backend,
SocketStream.aclose()closes the transport and thencheckpoints with
await sleep(0)before callingtransport.abort(). If thetransport's write buffer drains and
connection_lost()fires during thatcheckpoint, the transport has already detached itself from the event loop by
the time
aclose()resumes, so the subsequentabort()call raisesAttributeError: 'NoneType' object has no attribute 'call_soon'instead ofbeing the harmless no-op it's meant to be in that interleaving.
StreamProtocolnow records whetherconnection_lost()has already run, andaclose()skips theabort()call when it has, since the connection isalready gone at that point.
Added a deterministic regression test that schedules
connection_lost()tofire during the checkpoint (via
loop.call_soon) rather than relying on thetiming-dependent socket-buffer repro from the issue.
Checklist
tests/) which would fail without your patchdocs/), in case of behavior changes or newfeatures
docs/versionhistory.rst).